Skip to content

zig fmt: canonicalize nested cast builtin order #24199

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Aug 7, 2025
Merged

zig fmt: canonicalize nested cast builtin order #24199

merged 2 commits into from
Aug 7, 2025

Conversation

Justus2308
Copy link
Contributor

@Justus2308 Justus2308 commented Jun 16, 2025 •
edited
Loading

Closes #24106

Searches for surrounding tokens tagged with .builtin when a .builtin_... node whose main token ends in "Cast" is encountered and picks which one to render first according to the field order in CastKind. In case of a redundant casting operation (e.g. @ptrCast(@volatileCast(@ptrCast(...)))) no reordering is done for the entire nested chain.

The canonical ordering is the one proposed by mlugg:

@ptrCast(@alignCast(@addrSpaceCast(@constCast(@volatileCast(something)))))

It can be changed by simply rearranging the fields of CastKind (and adjusting the rendering order in translate_c.removeCVQualifiers for @constCast()/@volatileCast() and related tests).

The first commit is the actual implementation,the second commit is the new ordering applied to affected source files.

@Justus2308 Justus2308 marked this pull request as draft June 16, 2025 18:16
@Justus2308 Justus2308 force-pushed the 24106-fmt-casts branch 2 times, most recently from 3d1d91f to 0a86578 Compare June 16, 2025 21:06
@Justus2308 Justus2308 marked this pull request as ready for review June 16, 2025 21:33
Vexu
Vexu reviewed Jul 12, 2025
View reviewed changes
Justus2308
Justus2308 commented Jul 12, 2025
View reviewed changes
@mlugg
Copy link
Member

mlugg commented Jul 30, 2025

@Justus2308 what's the state of this PR? You've left a bunch of review comments which look like suggestions for yourself or something?

@Justus2308
Copy link
Contributor Author

Sorry, I'm not very familiar with the review process here/in general yet. Should I just apply the changes and add them in a separate commit here? Or should I amend my previous commits?

@mlugg
Copy link
Member

mlugg commented Jul 30, 2025

Feel free to commit however you prefer -- we can always manually do any fixups when we go to merge. If it's all the same to you, here I'd probably prefer you keep exactly two commits and amend them appropriately, but whatever you choose to do is fine!

@Justus2308 Justus2308 requested a review from Vexu July 30, 2025 20:15
mlugg
mlugg reviewed Jul 30, 2025
View reviewed changes
Copy link
Member

@mlugg mlugg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a proper review, just some light style suggestions

Vexu
Vexu approved these changes Aug 4, 2025
View reviewed changes
Copy link
Member

@Vexu Vexu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, currently this doesn't handle parentheses between the casts (@ptrCast((@constCast(something)))) but it should be straightforward to add if it is even desired.

@mlugg
Copy link
Member

mlugg commented Aug 4, 2025

FWIW I think it was a bad decision of mine to allow @ptrCast((@alignCast(x))) (with the redundant parentheses), I think the builtins should have to be directly nested. So I'm fine with the zig fmt canonicalization not handling that case.

@Justus2308
Copy link
Contributor Author

Great, thanks for the review! Can somebody please restart the cancelled CI job I'm 99% confident it will pass :)

@nektro
Copy link
Contributor

nektro commented Aug 4, 2025

Looks good, currently this doesn't handle parentheses between the casts (@ptrCast((@constCast(something)))) but it should be straightforward to add if it is even desired.

FWIW I think it was a bad decision of mine to allow @ptrCast((@alignCast(x))) (with the redundant parentheses), I think the builtins should have to be directly nested. So I'm fine with the zig fmt canonicalization not handling that case.

imo the long term solution is to make zig fmt also canonicalize parentheses usage

@Justus2308
Copy link
Contributor Author

Justus2308 commented Aug 4, 2025 •
edited
Loading

I agree, though #114 should probably be implemented first otherwise a canonicalization removing all technically unnecessary parentheses would make math expressions involving more than a couple of operators even harder to look at...

mlugg
mlugg requested changes Aug 7, 2025
View reviewed changes
Copy link
Member

@mlugg mlugg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting strategy -- took me a moment to properly understand -- but seems like it works fine. Thanks!

@mlugg
Copy link
Member

mlugg commented Aug 7, 2025

Oops, sorry, wrong button -- that was meant to be an "approve".

@mlugg mlugg merged commit fd9cfc3 into ziglang:master Aug 7, 2025
23 of 24 checks passed
@Justus2308 Justus2308 mentioned this pull request Aug 13, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mlugg mlugg mlugg requested changes

@Vexu Vexu Vexu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

zig fmt fails to canonicalize @ptrCast and @alignCast
4 participants
@Justus2308 @mlugg @nektro @Vexu